Fix size() and namespace lookups so that we can compile with C++17 #2641

SteveBronder · 2022-01-03T19:39:49Z

Summary

This adds math:: in front of calls to stan math's apply(), size(), and for_each() functions so that C++17's std functions are not used during ADL lookup. This should also fix the bug in #2474 so we can update to Eigen 3.4. This also makes all the size() functions return Eigen::Index so that we have a consistent definition of the size we use in stan math. If we don't have a consistent return for size() we can end up having compiler errors for functions like std::max({size_t, int, long int}) since there will be different integer types in the initializer list.

NOTE: I'm running this first with C++17 as the default std version in the makefiles just to make sure everything passes. Once we get a green light once I'm going to switch that back to std=c++1y

Tests

No new tests

Side Effects

One nice thing is that if we ever want to change the default index type we use we can just override Eigen::Index to be another integer type.

Release notes

Make stan math more easily compilable with C++17

Checklist

Math issue #(issue number)
Copyright holder: Steve Bronder

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- dependencies checks pass, (make test-math-dependencies)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

…4.1 (tags/RELEASE_600/final)

rok-cesnovar · 2022-01-04T09:52:07Z

Oh man, std::apply is also a thing :)

Minor Q, but should we just use the entire namespace (stan::math::) and not math::?

hsbadr · 2022-01-05T15:03:01Z

@SteveBronder This PR reintroduces the error fixed by #2632 (but at line 41 of scalar_seq_view.hpp), and requires the following minor patch:

diff --git a/stan/math/prim/fun/scalar_seq_view.hpp b/stan/math/prim/fun/scalar_seq_view.hpp
index 5e7a176b8f..7334b8b4f0 100644
--- a/stan/math/prim/fun/scalar_seq_view.hpp
+++ b/stan/math/prim/fun/scalar_seq_view.hpp
@@ -38,7 +38,7 @@ class scalar_seq_view<C, require_eigen_vector_t<C>> {

   template <typename T = C, require_st_arithmetic<T>* = nullptr>
   inline decltype(auto) val(size_t i) const {
-    return c_.coeffRef(i);
+    return c_.coeff(i);
   }

   template <typename T = C, require_st_autodiff<T>* = nullptr>

Here's a complete reproducible example:

remove.packages(c("StanHeaders", "rstan"))
remotes::install_github("stan-dev/rstan/StanHeaders@experimental", upgrade = "always", force = TRUE)
remotes::install_github("stan-dev/rstan/rstan/rstan@experimental", upgrade = "always", force = TRUE)

library(rstan)

stancode <- '
data {
  real<lower=0> shape;
  real<lower=0> rate;
}
parameters {
  real<lower=0> alpha[8];
}
model {
   target += gamma_lpdf(alpha | shape, rate);
}
'
mod <- stan_model(model_code = stancode, verbose = TRUE)

PS: Note that you need to merge this PR into StanHeaders to test it.

SteveBronder · 2022-01-06T18:43:09Z

@hsbadr Thanks, done!

hsbadr · 2022-01-06T19:33:43Z

@hsbadr Thanks, done!

Thank you! I confirm that the above model is successfully compiled after c5e7916. I'll let you know if I run into any related issue.

SteveBronder · 2022-01-11T15:25:38Z

@rok-cesnovar very odd, on develop if I add the CXXFLAGS+= -fsanitize=memory and run

./runTests.py -j24 ./test/unit/math/rev/core/accumulate_adjoints_test.cpp

I'm getting the same error as jenkins with this PR? It looks like it's something deep in gtest and nothing to do with us?

==529809==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x537339 in testing::internal::FilePath::Normalize() /home/steve/stan/origin/math/lib/benchmark_1.5.1/googletest/googletest/src/gtest-filepath.cc:358:16
    #1 0x540ee6 in testing::internal::FilePath::FilePath(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/steve/stan/origin/math/lib/benchmark_1.5.1/googletest/googletest/include/gtest/internal/gtest-filepath.h:68:5
    #2 0x536403 in testing::internal::FilePath::GetCurrentDir() /home/steve/stan/origin/math/lib/benchmark_1.5.1/googletest/googletest/src/gtest-filepath.cc:112:10
    #3 0x546327 in testing::internal::UnitTestImpl::AddTestInfo(void (*)(), void (*)(), testing::TestInfo*) /home/steve/stan/origin/math/lib/benchmark_1.5.1/googletest/googletest/src/gtest-internal-inl.h:693:33
    #4 0x510b1b in testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, testing::internal::CodeLocation, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*) /home/steve/stan/origin/math/lib/benchmark_1.5.1/googletest/googletest/src/gtest.cc:2770:22
    #5 0x423530 in __cxx_global_var_init.14 /home/steve/stan/origin/math/test/unit/math/rev/core/accumulate_adjoints_test.cpp:6:1
    #6 0x426c11 in _GLOBAL__sub_I_accumulate_adjoints_test.cpp /home/steve/stan/origin/math/test/unit/math/rev/core/accumulate_adjoints_test.cpp
    #7 0x575f2c in __libc_csu_init (/home/steve/stan/origin/math/test/unit/math/rev/core/accumulate_adjoints_test+0x575f2c)
    #8 0x7f39bee9f03f in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:264:6
    #9 0x4282fd in _start (/home/steve/stan/origin/math/test/unit/math/rev/core/accumulate_adjoints_test+0x4282fd)

This seems to happen when I tested it on clang-12 and 10

SteveBronder · 2022-01-11T15:28:24Z

~~Oh sorry ignore the above that's a different error with -fsanitize=memory. I can't replicate the wild pointer thing locally?~~

Lol nope nvm I got it sorry

…4.1 (tags/RELEASE_600/final)

…ow-c17

…4.1 (tags/RELEASE_600/final)

stan-buildbot · 2022-01-14T07:49:07Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.57	3.6	0.99	-0.88% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.21% slower
eight_schools/eight_schools.stan	0.09	0.09	1.01	1.12% faster
gp_regr/gp_regr.stan	0.14	0.14	1.02	1.63% faster
irt_2pl/irt_2pl.stan	5.73	5.72	1.0	0.24% faster
performance.compilation	93.26	90.28	1.03	3.19% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.12	8.1	1.0	0.16% faster
pkpd/one_comp_mm_elim_abs.stan	30.78	30.95	0.99	-0.56% slower
sir/sir.stan	122.48	130.44	0.94	-6.5% slower
gp_regr/gen_gp_data.stan	0.03	0.03	1.0	-0.01% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.01	3.03	0.99	-0.91% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.37	0.4	0.95	-5.59% slower
arK/arK.stan	2.09	2.1	0.99	-0.64% slower
arma/arma.stan	0.28	0.28	0.99	-0.99% slower
garch/garch.stan	0.61	0.61	1.0	-0.47% slower
Mean result: 0.9923386527

Jenkins Console Log
Blue Ocean
Commit hash: 22034ef

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

andrjohns · 2022-01-14T10:09:12Z

Anything else to go into this one? I can review when it's ready

andrjohns · 2022-03-27T14:14:10Z

@serban-nicusor-toptal I'm running into the leak sanitizer CI issues with this PR that haven't been resolved by the switch to Docker and that I can't reproduce locally - even when I run the same commands in the same docker image (stanorg/ci:gpu):

test/unit/math_c11_test --gtest_output="xml:test/unit/math_c11_test.xml"
Running main() from lib/benchmark_1.5.1/googletest/googletest/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from MathMeta
[ RUN      ] MathMeta.ensure_c11_features_present
[       OK ] MathMeta.ensure_c11_features_present (0 ms)
[----------] 1 test from MathMeta (0 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 1 test.
==14091==LeakSanitizer has encountered a fatal error.
==14091==HINT: For debugging, try setting environment variable LSAN_OPTIONS=verbosity=1:log_threads=1
==14091==HINT: LeakSanitizer does not work under ptrace (strace, gdb, etc)
test/unit/math_c11_test --gtest_output="xml:test/unit/math_c11_test.xml" failed
exit now (03/27/22 09:52:55 EDT)
script returned exit code 1

When you've got a minute, would you mind having a look to see if anything obvious stands out or if you're able to reproduce?

serban-nicusor-toptal · 2022-03-28T08:36:24Z

I will run some tests and try to see what's different.
Could this be affected by the CPU? If I remember right we had some issues with OpenCL tests because some instructions are not supported by the CPU.
You can also take a look at the CI Dockerfile maybe something stands out?
I see that others PRs are just fine so I think something is/needs to be different here.

rok-cesnovar · 2022-03-28T08:49:56Z

We probably don't need the test that is causing the sanitizer failures, though it doesn't seem like it should cause issues here.

andrjohns · 2022-03-28T09:58:26Z

The strange thing is that it doesn't fail when I run the tests in the docker image locally, so I'm not sure what's different about the Jenkins setup

andrjohns · 2022-03-28T11:09:38Z

I'll also try splitting this out into smaller PRs to see if I can narrow down the issue, and also make it easier to review and merge

serban-nicusor-toptal · 2022-03-28T13:25:11Z

It might be either a permission issue or a hardware component.

andrjohns · 2022-06-22T09:53:21Z

@SteveBronder now that #2693 has merged the namespace qualifiers, I'm going to close this PR and open a separate PR for just the size() return type changes (I've also made a reference issue in #2760).

Let me know if there was anything else in this PR that would be good to add in another PR!

SteveBronder and others added 9 commits January 3, 2022 14:34

fix ADL lookups and size() functions to all return same integral type

a31c257

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

418a497

…4.1 (tags/RELEASE_600/final)

update scalar_seq_view<T*>.size() to return Eigen::Index

a51deac

Fix apply includes

8819e8e

update to use Rtools 4.0

4f4e954

fix includes for opencl

26a5dc8

fix size namespace calls in test

470b62d

Merge commit '46fa82de5489d0ddacae6bb33d4c450bde5eb939' into HEAD

9778fa6

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

39402ca

…4.1 (tags/RELEASE_600/final)

fix namespace qualifier for apply in cvodes_integrator

a169c6c

SteveBronder added 2 commits January 6, 2022 12:43

Merge remote-tracking branch 'origin/develop' into fix/allow-c17

7a36db0

fix scalar_seq_view val() compiler error

c5e7916

fix ng_binomial_2_lccdf

fdd8f69

SteveBronder and others added 11 commits January 11, 2022 10:44

update profiling to use forwarding for string value

b31c38e

Merge commit '1a447f26424a80f4cb5a053a1215a1a63c8b1ca6' into HEAD

bfc7cd9

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

0be15ed

…4.1 (tags/RELEASE_600/final)

fix some missing size() values

6c9a9fb

Merge branch 'fix/allow-c17' of github.com:stan-dev/math into fix/all…

f5b3c10

…ow-c17

update fvar ops and partials

ab6a234

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

e187555

…4.1 (tags/RELEASE_600/final)

update fvar operands and partials

9fb0885

fixes for neg_binomial_2

5f1c891

Merge commit 'e15c69f777647c0884ab1ac807c9be9101cef641' into HEAD

8b67d21

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

7079421

…4.1 (tags/RELEASE_600/final)

SteveBronder added 2 commits January 12, 2022 13:03

fixes for neg_binomial_2

c583774

fix expression test includes

22034ef

SteveBronder and others added 6 commits March 1, 2022 15:43

Merge remote-tracking branch 'origin/develop' into fix/allow-c17

99c5fed

Merge remote-tracking branch 'upstream/develop' into fix/allow-c17

16a1b01

Merge remote-tracking branch 'upstream/develop' into fix/allow-c17

4a0a164

Update namespace qualifiers

d2a9a8b

Trigger CI

15a9477

Merge remote-tracking branch 'upstream/develop' into fix/allow-c17

242e440

hsbadr mentioned this pull request Mar 27, 2022

[WIP] Update to Eigen 3.4 #2583

Merged

5 tasks

Test clang for leak-sanitizer

3d43abf

andrjohns mentioned this pull request Mar 28, 2022

C++17 prep - namespace qualifiers only #2693

Merged

5 tasks

andrjohns mentioned this pull request Jun 22, 2022

Consistent return type for size() #2760

Open

andrjohns closed this Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix size() and namespace lookups so that we can compile with C++17 #2641

Fix size() and namespace lookups so that we can compile with C++17 #2641

SteveBronder commented Jan 3, 2022

rok-cesnovar commented Jan 4, 2022

hsbadr commented Jan 5, 2022

SteveBronder commented Jan 6, 2022

hsbadr commented Jan 6, 2022

SteveBronder commented Jan 11, 2022

SteveBronder commented Jan 11, 2022 •

edited

Loading

stan-buildbot commented Jan 14, 2022

andrjohns commented Jan 14, 2022

andrjohns commented Mar 27, 2022

serban-nicusor-toptal commented Mar 28, 2022 •

edited

Loading

rok-cesnovar commented Mar 28, 2022

andrjohns commented Mar 28, 2022

andrjohns commented Mar 28, 2022

serban-nicusor-toptal commented Mar 28, 2022

andrjohns commented Jun 22, 2022

Fix size() and namespace lookups so that we can compile with C++17 #2641

Fix size() and namespace lookups so that we can compile with C++17 #2641

Conversation

SteveBronder commented Jan 3, 2022

Summary

Tests

Side Effects

Release notes

Checklist

rok-cesnovar commented Jan 4, 2022

hsbadr commented Jan 5, 2022

SteveBronder commented Jan 6, 2022

hsbadr commented Jan 6, 2022

SteveBronder commented Jan 11, 2022

SteveBronder commented Jan 11, 2022 • edited Loading

stan-buildbot commented Jan 14, 2022

andrjohns commented Jan 14, 2022

andrjohns commented Mar 27, 2022

serban-nicusor-toptal commented Mar 28, 2022 • edited Loading

rok-cesnovar commented Mar 28, 2022

andrjohns commented Mar 28, 2022

andrjohns commented Mar 28, 2022

serban-nicusor-toptal commented Mar 28, 2022

andrjohns commented Jun 22, 2022

SteveBronder commented Jan 11, 2022 •

edited

Loading

serban-nicusor-toptal commented Mar 28, 2022 •

edited

Loading